Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add S.fromEither to pull out values from either #229

Merged
merged 1 commit into from
Jun 7, 2016
Merged

Add S.fromEither to pull out values from either #229

merged 1 commit into from
Jun 7, 2016
+59 −0

Conversation

wennergr
Copy link
Contributor

@wennergr wennergr commented Jun 7, 2016

S.fromEither :: b -> Either a b -> b

Closes: #130

@wennergr wennergr force-pushed the feature-S.fromEither branch from d269f79 to fea90f9 Compare June 7, 2016 03:21
@davidchambers
Copy link
Member

Thank you, @wennergr! This was a nice surprise. :)

@davidchambers
Copy link
Member

The order in which the functions appear in the source file determines the order in which the functions are listed in the documentation, so it's nice to group related functions. In this case, let's follow the (somewhat arbitrary) precedent set by fromMaybe—which appears after isJust and before maybe—by inserting fromEither between isRight and either.

davidchambers
davidchambers reviewed Jun 7, 2016
View reviewed changes
test/fromEither.js

it('type checks its arguments', function() {
throws(function() { S.fromEither(0, [1, 2, 3]); },
'Invalid value\n' +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's adjust the indentation like so:

throws(function() { S.fromEither(0, [1, 2, 3]); },
       'Invalid value\n' +

@wennergr
Copy link
Contributor Author

wennergr commented Jun 7, 2016

Thanks for your feedback

I did not do property based tests because the fromMaybe tests didn't. Is it only used to test laws? Performance reasons? More then happy to change ;)

Also more then happy to rebase before the merge.

@davidchambers
Copy link
Member

I did not do property based tests because the fromMaybe tests didn't. Is it only used to test laws?

The truthful answer is that property-based testing is still relatively new to me, so I don't think to use it. While it would be great to add more property-based tests, that's a larger change orthogonal to this one.

Also more then happy to rebase before the merge.

That would be marvellous. :)

I'd like to add that I ❤️ the way in which you went about this pull request. You matched the existing code and documentation style, and referenced the issue this pull request will close. Very nice!

@wennergr
Add S.fromEither to pull out values from either
be1f26e 
S.fromEither :: b -> Either a b -> b

Closes: sanctuary-js#130
@wennergr wennergr force-pushed the feature-S.fromEither branch from 8ce285c to be1f26e Compare June 7, 2016 06:50
@wennergr
Copy link
Contributor Author

wennergr commented Jun 7, 2016

Done, and thanks!

@davidchambers
Copy link
Member

🌳

@davidchambers davidchambers merged commit bde77ac into sanctuary-js:master Jun 7, 2016
@wennergr wennergr deleted the feature-S.fromEither branch June 7, 2016 21:01
@davidchambers davidchambers mentioned this pull request Dec 17, 2016
Closed
@davidchambers davidchambers mentioned this pull request Feb 13, 2017
Closed
@davidchambers
Copy link
Member 

$ npm install [email protected]

$ node
> const S = require('sanctuary')
undefined
> S.fromEither
fromEither :: b -> Either a b -> b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wennergr @davidchambers